[React 18] Fix cross-version compatibility and unit test errors by tkajtoch · Pull Request #6994 · elastic/eui

tkajtoch · 2023-07-25T21:34:42Z

Summary

This PR fixes all remaining unit tests, adds cross-version compatibility to useRenderToText() and fixes code style in a few files I missed that were merged before (sorry!!)

QA

Checkout this branch
Run rm -rf node_modules && yarn
Run yarn lint
Run yarn test-unit
Run REACT_VERSION=17 yarn test-unit
Confirm all of the above succeeded

src/components/inner_text/render_to_text.tsx

kibanamachine · 2023-07-25T21:58:10Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_6994/

kibanamachine · 2023-07-26T09:39:52Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_6994_buildkite/

kibanamachine · 2023-07-26T09:40:05Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_6994/

cee-chen · 2023-07-26T15:04:47Z

@tkajtoch CI is still failing with a lint error it looks like:

02:28:31 /app/src/components/combo_box/combo_box.test.tsx
02:28:31   10:27  error  '/app/node_modules/@testing-library/react/dist/@testing-library/react.esm.js' imported multiple times  import/no-duplicates
02:28:31   13:21  error  '/app/node_modules/@testing-library/react/dist/@testing-library/react.esm.js' imported multiple times  import/no-duplicates

scripts/jest/setup/throw_on_console_error.js

cee-chen · 2023-07-26T15:10:50Z

src/test/rtl/render_hook.ts

+} else {
+  // eslint-disable-next-line @typescript-eslint/no-var-requires
+  renderHook = require('@testing-library/react-hooks').renderHook;
+  // eslint-disable-next-line @typescript-eslint/no-var-requires


Let's just // eslint-disable @typescript-eslint/no-var-requires the entire file instead of having 4 separate comments, please!

Also oof, it's unfortunate that this is necessary - I'm curious as to what on earth changed in React 18 that hooks render differently compared in 17 🤦

The change from import { render } from 'react-dom' to import { createRoot } from 'react-dom/client' caused all this :(

Gotcha, thanks for clarifying!

I wondered why // eslint-disable @typescript-eslint/no-var-requires didn't work for me yesterday... you have to use /* */ for multi-line disables instead 🤦🏻‍♂️

It's fixed now!

tkajtoch · 2023-07-26T15:18:15Z

cypress/support/setup/mount.tsx

  cypressMount = require('@cypress/react').mount;
 }

-const mountCommand = (children: ReactNode): ReturnType<typeof mount> => {


@cee-chen I missed this when I was rebasing feature/react-18 a few days ago. The filename changed from mount.js to mount.tsx and I didn't catch your changes to reapply to the new file.

Nice, thanks for catching that!

kibanamachine · 2023-07-26T15:37:52Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_6994_buildkite/

kibanamachine · 2023-07-26T15:41:08Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_6994/

scripts/jest/config.js

cee-chen · 2023-07-26T15:45:01Z

src/test/react_version.ts

+export const testOnReactVersion = (
+  versionOrVersions: ReactVersion | ReactVersion[]
+): typeof test => {
+  return isReactVersion(versionOrVersions) ? test : test.skip;


This is a super elegant util 👏 I wish I knew why that specific EuiOverlayMask test was failing though. Is this an RTL-specific issue that they might fix on their end later, or do we need to rewrite our test?

I think it's connected to how React 18 RTL and jest interact and I hope it can be fixed on their end, we'll see when upgrading to the latest jest. It might also be related to throwing an error during a react component update that throws out react internals.

I tried wrapping <EuiOverlayMask aria-hidden={true} /> in class component with componentDidCatch, rendering it differently, moving the throw statement outside useEffect and none seemed to help.

I created #6998 to track this.

Awesome, thanks so much for that info - that's super helpful!

cee-chen · 2023-07-26T15:54:01Z

src/components/datagrid/controls/column_sorting_draggable.test.tsx

These are seriously fantastic test conversions - thanks for preferring RTL queries over snapshots as always!

…React 18

kibanamachine · 2023-07-26T16:52:57Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_6994_buildkite/

kibanamachine · 2023-07-26T16:54:50Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_6994_buildkite/

kibanamachine · 2023-07-26T17:12:11Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_6994_buildkite/

kibanamachine · 2023-07-26T17:26:50Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_6994_buildkite/

kibanamachine · 2023-07-26T17:27:10Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_6994/

cee-chen · 2023-07-26T17:55:31Z

1 last test failure on CI!!

10:33:39 FAIL src/components/inline_edit/inline_edit_form.test.tsx
10:33:39   ● Test suite failed to run
10:33:39 
10:33:39     Call retries were exceeded
10:33:39 
10:33:39       at ChildProcessWorker.initialize (node_modules/jest-worker/build/workers/ChildProcessWorker.js:193:21)
10:33:39 
10:33:39 
10:33:39 Test Suites: 1 failed, 1 skipped, 392 passed, 393 of 394 total

cee-chen · 2023-07-26T18:05:16Z

src/components/datagrid/controls/keyboard_shortcuts.test.tsx

-    act(() => getByTestSubject('dataGridKeyboardShortcutsButton').click());
-    rerender(<>{result.current.keyboardShortcuts}</>);
+    renderHookAct(() =>
+      getByTestSubject('dataGridKeyboardShortcutsButton').click()


So, unfortunately this test is no longer working/outputting the expected DOM element - the snapshot is missing the portal content.

Can you try using fireEvent.click here instead of .click()?

Great catch, thank you!

cee-chen · 2023-07-26T18:06:20Z

src/components/icon/icon.test.tsx

+  });
+
+  afterAll(() => {
+    consoleErrorOverride.mockRestore();


This looks fantastic, thanks so much for making this change!

kibanamachine · 2023-07-26T18:33:34Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_6994_buildkite/

kibanamachine · 2023-07-26T18:34:15Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_6994/

cee-chen · 2023-07-26T19:26:47Z

jenkins test this

…ments

kibanamachine · 2023-07-26T20:11:02Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_6994/

kibanamachine · 2023-07-26T20:12:10Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_6994_buildkite/

cee-chen

🎉 Hopefully Jenkins doesn't lose its mind this time and CI finally passes green!

elasticmachine · 2023-07-26T20:24:07Z

💚 Build Succeeded

Buildkite Build
Commit: 3de0ccc

History

💚 Build #79 succeeded 9ff40cb
💚 Build #74 succeeded f350136
💚 Build #73 succeeded 71dc3e2
💚 Build #72 succeeded 3974d37
💚 Build #71 succeeded caba775
💚 Build #69 succeeded 0b93e9e

cc @tkajtoch

tkajtoch · 2023-07-26T21:30:29Z

jenkins test this please

kibanamachine · 2023-07-26T21:54:38Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_6994/

tkajtoch requested review from a team and cee-chen July 25, 2023 21:34

tkajtoch self-assigned this Jul 25, 2023

tkajtoch commented Jul 25, 2023

View reviewed changes

src/components/inner_text/render_to_text.tsx Outdated Show resolved Hide resolved

tkajtoch force-pushed the feature/react-18 branch from 2d13686 to e5f68aa Compare July 26, 2023 07:36

test: ignore EuiIcon "update not wrapped in act()"

cd88957

tkajtoch force-pushed the feat/react-18-fix-compatibility-and-unit-tests branch from 3110b4c to cf26cf3 Compare July 26, 2023 09:17

cee-chen reviewed Jul 26, 2023

View reviewed changes

scripts/jest/setup/throw_on_console_error.js Outdated Show resolved Hide resolved

cee-chen reviewed Jul 26, 2023

View reviewed changes

tkajtoch commented Jul 26, 2023

View reviewed changes

cee-chen reviewed Jul 26, 2023

View reviewed changes

scripts/jest/config.js Outdated Show resolved Hide resolved

cee-chen reviewed Jul 26, 2023

View reviewed changes

tkajtoch force-pushed the feat/react-18-fix-compatibility-and-unit-tests branch from caba775 to 3974d37 Compare July 26, 2023 16:31

tkajtoch added 10 commits July 26, 2023 18:49

test: import renderHook conditionally based on version of React used

a1078f6

test: convert EuiDataGridColumnSortingDraggable tests to RTL

d74349f

test: skip "throws if a non-string property value is passed" test on …

1c3494d

…React 18

test: update snapshots

863a643

refactor: update useRenderToText to support React 18 and below

bcf4893

test: wrap EuiComboBox options dropdown test setState() in act()

fd80b55

style: fix lint errors

6896559

feat: remove unused useRenderToText internal utility

78a0187

fix: restore cypress mount() support for passing providerProps

72fc892

fix: duplicate @testing-library/react import in combo_box.test.tsx

71dc3e2

tkajtoch force-pushed the feat/react-18-fix-compatibility-and-unit-tests branch from 3974d37 to 71dc3e2 Compare July 26, 2023 16:49

fix: move EuiIcon console.error handler to icon.test.tsx

f350136

cee-chen reviewed Jul 26, 2023

View reviewed changes

fix: await waitFor calls in "handles async promises" test

9ff40cb

fix: snapshot baseElement instead of wrapper to include portalled ele…

3de0ccc

…ments

cee-chen approved these changes Jul 26, 2023

View reviewed changes

tkajtoch closed this Jul 26, 2023

tkajtoch mentioned this pull request Jul 26, 2023

[React 18] Fix cross-version compatibility and unit test errors #7002

Merged

Conversation

tkajtoch commented Jul 25, 2023

Summary

QA

Uh oh!

Uh oh!

kibanamachine commented Jul 25, 2023

Uh oh!

kibanamachine commented Jul 26, 2023

Uh oh!

kibanamachine commented Jul 26, 2023

Uh oh!

cee-chen commented Jul 26, 2023

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kibanamachine commented Jul 26, 2023

Uh oh!

kibanamachine commented Jul 26, 2023

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kibanamachine commented Jul 26, 2023

Uh oh!

kibanamachine commented Jul 26, 2023

Uh oh!

kibanamachine commented Jul 26, 2023

Uh oh!

kibanamachine commented Jul 26, 2023

Uh oh!

kibanamachine commented Jul 26, 2023

Uh oh!

cee-chen commented Jul 26, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kibanamachine commented Jul 26, 2023

Uh oh!

kibanamachine commented Jul 26, 2023

Uh oh!

cee-chen commented Jul 26, 2023

Uh oh!

kibanamachine commented Jul 26, 2023

Uh oh!

kibanamachine commented Jul 26, 2023

Uh oh!

cee-chen left a comment

Choose a reason for hiding this comment

Uh oh!

elasticmachine commented Jul 26, 2023

💚 Build Succeeded

History

Uh oh!

tkajtoch commented Jul 26, 2023

Uh oh!

kibanamachine commented Jul 26, 2023

Uh oh!

Reviewers